Refactoring URLSession for multiple native protocols #1216

saiHemak · 2017-09-14T10:04:01Z

The code in this PR is a part of #1122 and has been separated to reduce the size of the code change.

#968 refactored URLSession to decouple it from the HTTP implementation and allow native protocols to be implemented as URLProtocols, uniform with custom protocols. With this PR, we go a step further, and factor out common code which can be used with multiple native protocols. All of this code is moved to a new class called _NativeProtocol.

The existing HTTP implementation and the FTP Implementation [#1122] will be reusing _NativeProtocol.

saiHemak · 2017-09-14T10:04:29Z

@pushkarnk please review

weissi

I don't understand lots about this but had a quick look and left a few comments.

The code itself seems to be mostly moved from other files.

weissi · 2017-09-14T12:57:41Z

Foundation/URLSession/BodySource.swift

-            fatalError("Cant create DispatchIO channel")
+                                       cleanupHandler: {_ in })
+            else {
+                fatalError("Cant create DispatchIO channel")


this will fail if the file doesn't exist, why crash?

weissi · 2017-09-14T13:02:36Z

Foundation/URLSession/NativeProtocol.swift

+
+class _NativeProtocol: URLProtocol, _EasyHandleDelegate {
+    internal var easyHandle: _EasyHandle!
+    internal var totalDownloaded = 0


can't see this being used anywhere, needed?

weissi · 2017-09-14T13:03:14Z

Foundation/URLSession/NativeProtocol.swift

+}()
+
+class _NativeProtocol: URLProtocol, _EasyHandleDelegate {
+    internal var easyHandle: _EasyHandle!


this doesn't seem to get modified, let?

easyHandle is getting initialized during multiple init and hence the compiler complains Immutable value 'self.easyHandle' may only be initialized once

weissi · 2017-09-14T13:11:18Z

Foundation/URLSession/NativeProtocol.swift

+    internal var easyHandle: _EasyHandle!
+    internal var totalDownloaded = 0
+    internal lazy var tempFileURL: URL = {
+        let fileName = NSTemporaryDirectory() + NSUUID().uuidString + ".tmp"


don't we have a better/safer way to create temp files somewhere

is NSTemporaryDirectory() guaranteed to end in a / ?

This change has been incorporated into the code base through #1195 I have just refactored and pulled it to NativeProtocol as it can be used by multiple protocols like FTP and HTTP

saiHemak · 2017-09-15T05:02:16Z

@weissi -Thanks for the review. Yes its just refactoring of URLSession code .Earlier the code was tightly coupled with HTTP Protocol.To implement the support for other protocols like FTP I moved out the common code in to Native protocol .Please refer to the #1122 for more details on how this code will be used to support FTP and HTTP protocols.

pushkarnk · 2017-09-15T10:15:01Z

@swift-ci please test

pushkarnk · 2017-09-15T10:15:34Z

@swift-ci please test

ianpartridge · 2017-09-15T10:50:47Z

I know this is an inherited problem and not caused by this PR, but the URLSession code is littered with fatalError() calls which I think would be better replaced by assert() or preconditionFailure(). What do you think @pushkarnk?

alblue · 2017-09-15T11:07:11Z

Yeah, after perusing #1122 I have the same concern :-) I think it's a good time to bring it up to date with coding practices.

ianpartridge · 2017-09-15T10:47:05Z

Foundation/URLSession/NativeProtocol.swift

+        return .proceed
+    }
+
+    func validateHeaderComplete(transferSate: _TransferState) -> URLResponse? {


Typo: transferSate -> transferState

ianpartridge · 2017-09-15T10:53:44Z

Foundation/URLSession/NativeProtocol.swift

+    }
+
+    fileprivate func notifyDelegate(aboutReceivedData data: Data) {
+        guard let t = self.task else { fatalError("Cannot notify") }


This creates t for the task but doesn't really use it. For example, the next line could be if case .taskDelegate(let delegate) = t.session.behaviour(for: t),

ianpartridge · 2017-09-15T11:01:15Z

Foundation/URLSession/http/HTTPURLProtocol.swift

+    /// What action to take
+    override func completionAction(forCompletedRequest request: URLRequest, response: URLResponse) -> _CompletionAction {
+        // Redirect:
+        let httpURLResponse = response as!  HTTPURLResponse


The pattern used elsewhere in the codebase is

guard let response = response as? HTTPURLResponse else { fatalError("Reponse was not HTTPURLResponse") }

ianpartridge · 2017-09-15T11:04:51Z

Foundation/URLSession/http/HTTPURLProtocol.swift

+        //TODO: Should keep track of the number of redirects that this
+        // request has gone through and err out once it's too large, i.e.
+        // call into `failWith(errorCode: )` with NSURLErrorHTTPTooManyRedirects
+        guard case .transferCompleted(response: let response, bodyDataDrain: let bodyDataDrain) = self.internalState else {


response and bodyDataDrain don't seem to be used?

Its been used .

ianpartridge · 2017-09-15T11:07:38Z

Foundation/URLSession/NativeProtocol.swift

+        let url: URL
+        /// Raw headers received.
+        let parsedResponseHeader: _ParsedResponseHeader
+        /// Once the headers is complete, this will contain the response


is -> are

ianpartridge · 2017-09-15T11:08:09Z

Foundation/URLSession/NativeProtocol.swift

+        let requestBodySource: _BodySource?
+        /// Body data received
+        let bodyDataDrain: _NativeProtocol._DataDrain
+        /// Describes what to do with received body data for this transfer:


What is this comment for?

ianpartridge · 2017-09-15T11:17:26Z

Foundation/URLSession/NativeProtocol.swift

+        /// The transfer completed.
+        ///
+        /// The easy handle has been removed from the multi handle. This does
+        /// not (necessarily mean the task completed. A task that gets


ianpartridge · 2017-09-15T11:19:45Z

I've done a first pass but feel like I only scratched the surface of this PR. There is so much that can be improved - not a criticism of @saiHemak who is working valiantly!

pushkarnk · 2017-09-18T12:25:52Z

@ianpartridge Yes, there's a lot of cleanup needed, with the fatalError() calls and in general. However, I am not sure if we should add that here. I'd prefer that done in a different (set of) pull request(s). What do you think?

ianpartridge · 2017-09-18T12:45:01Z

Yes, we should split up the work as much as possible.

johnno1962 · 2017-10-07T16:37:52Z

Hi, would it be possible to avoid merging this refactoring before #1198? Merging in the opposite order will involve a good deal more “conflicts”.

saiHemak · 2017-10-10T05:41:24Z

@ianpartridge @pushkarnk please review ..

pushkarnk · 2017-10-13T06:35:43Z

Foundation/URLSession/NativeProtocol.swift

+
+    fileprivate func notifyDelegate(aboutReceivedData data: Data) {
+        guard let t = self.task else {
+            fatalError("Cannot notify") }


The } needs to be indented right.

pushkarnk · 2017-10-13T06:36:48Z

Foundation/URLSession/NativeProtocol.swift

+
+    func fill(writeBuffer buffer: UnsafeMutableBufferPointer<Int8>) -> _EasyHandle._WriteBufferResult {
+        guard case .transferInProgress(let ts) = internalState else {
+            fatalError("Requested to fill write buffer, but transfer isn't in progress.") }


I think we need to have the } on the next line

saiHemak · 2017-10-31T06:20:06Z

@xwu , Thanks for your time in reviewing this .I agree that this would take considerable amount of time to review as it cannot be directly compared with old diffs. However, as explained in the description this PR is all about refactoring the existing code to support multiple native protocols like FTP,HTTP ... Hence most of the code is same and it is just a movement from one file to other file. Refactoring is needed because FTP also reuses the most of the code written for HTTP like header parsing,code used for transfer.For more details, please refer to PR #1122 - Initial implementation to support FTP re-uses the same code.

xwu · 2017-11-01T03:03:52Z

@saiHemak For reviewers' benefit, can you please document explicitly, then, which code was moved from where, and what code is new? This could be done in text, but it would also be sufficient to have two commits here, one only moving existing code--with no additions whatsoever, maintaining all existing comments and formatting--then another commit with your changes to the code.

saiHemak · 2017-11-02T11:50:18Z

@xwu

Thanks for the inputs ..

Actually, though the code is just a movement, few required the renaming too ..
So I am looking for a way on how can I split the commits ..

Here are the details of refactoring and reason behind refactoring ..

Moved lib curl specific code [libcurlHelpers.swift,EasyHandle.swift and MultiHandle.swift] from URLSession/http to URLSession/libcurl
Reason : libcurl specific code is common and is not specific to HTTP .Hence moved out of the HTTP folder to a common licurl folder  
Created a class NativeProtocol which basically holds the common implementation of native protocols like HTTP , FTP and so on .It implements the URLProtocol and EashHandleDelegate .
Reason: Need of a common class which holds the basic functionality and can be extended by respective protocol implementations.
Moved enableLibcurlDebugOutput and enableDebugOutput debug flags to NativeProtocol .These can be enabled on EasyHandle irrespective of the protocol. 
Moved _HTTPURLProtocol.InternalState from HTTPURLProtocol to _NativeProtocol.InternalState .Moved inits from HTTPURLProtocol to NativeProtocol
Reason:  Inits are initializing common variables like easyHandle,internal state and temporary file .
 5. Modified HTTPURLProtocol to extend from _NativeProtocol and inits to just call super.init()

  6. Moved easyHandle and tempFileURL instance variables to NativeProtocol

 7. Moved implemented methods _HTTPURLProtocol: _EasyHandleDelegate .
Reason :

EasyHandleDelegate methods provides functionality for transfer irrespective of protocol to NativeProtocol-

fill(writeBuffer buffer: UnsafeMutableBufferPointer) ,
notifyDelegate(aboutUploadedData),
notifyDelegate(aboutReceivedData),
didReceive(data).
transferCompleted(withError error: NSError?)
createTransferState()
createTransferBodyDataDrain

However few methods like below are protocol specific hence have been overridden in respective protocol classes .

didReceive(headerData data: Data, contentLength: Int64)
 Though the headers basically ends with \r\n irrespective of protocol the header format in FTP is different from HTTP. For e.g.: Headers in FTP looks like a plain text header with status code and description unlike the key-value pair headers in HTTP

8.NativeProtocol has definition for configureEasyHandle(easyHandle) which needs concrete implementation which is protocol specific by sub-classes like FTP and HTTP.
Reason:
HTTP requires some more additional configurations like adding the headers, setting allowed protocols. 
9.Renamed the HTTPBodySource to BodySource and moved out of the HTTP folder.
 Reason: The code respective to data handling is same irrespective to protocol   10. HTTPMessage._HTTPCharacters has been moved and renamed to _NativeProtocol.Delimiters Reason: Refer to next point 

Moved HTTPURLProtocol. _ParsedResponseHeader from HTTPMessage.swift to NativeProtocol.swift under _NativeProtocol._ParsedResponseHeader .
Reason:  This implementation basically does header parsing based on \r\n delimiters. Except the fact that empty header is received in case of HTTP to mark as Header completion and 150 status marks the open of data communication in FTP.  12. Moved and renamed HTTPURLProtocol._HTTPTransferState from TransferState.swift to _Nativeprotocol.TransferState . Moved related

Reason: Moved the code to Native Protocol as Transfer states have 1-to-1 with EasyHandle and irrespective of protocol

As the complete TransferState code has been aligned to Native protocol TransferState.swift file has been removed .

New Implementation :

NativeProtocol.validateHeaderComplete
Reason :
Should not throw error incase of simple-responseshttps://github.com//pull/1097

isCompleteHeader -> Inner method
Reason:

Refer to point 11 reason.In order to support both protocols this method has been introduced
-> which returns to true when the header is empty in case of HTTP.
-> Returns true if the header starts with 150

alblue · 2017-11-02T16:16:31Z

It would be good to split the commits, with each commit message outlining the points you have made above, so that anyone reviewing the changes afterwards would have the context without having to go back to a GitHub PR to try and understand the why. It would also help reviewing the changes independently and validate each one separately, including that all of the moves are done without changing code (for sanity) and to then ensure that such moves continue to be built via the PR testing mechanism, instead of just the end result.

Moved lib curl specific code [libcurlHelpers.swift,EasyHandle.swift and MultiHandle.swift] from URLSession/http to URLSession/libcurl libcurl abstractions like EasyHandle and MultiHandle are not specific to HTTP.Hence moved out of the HTTP folder to a common licurl folder

…ectory Added libcurl abstractions to libcurl directory

The code respective to data handling is same irrespective to protocol.So moving it under URLSession folder.

Moved under URLSession as the code related to data handling like read backed up by a file or buffer is common to all protocols.

Transfer State has 1-0-1 relationship with Easy handle and is not bound to any protocol.So moved the code to NativeProtocol and renamed to _NativeProtocol. TransferState instead of HTTPTranserState

Created a class NativeProtocol which basically holds the common implementation of native protocols like HTTP , FTP and so on . It implements the URLProtocol and EashHandleDelegate.Moved enableLibcurlDebugOutput and enableDebugOutput debug flags from HTTPURLProtocol to NativeProtocol as these can be enabled on EasyHandle irrespective of the protocol.Moved _HTTPURLProtocol.InternalState from HTTPURLProtocol to _NativeProtocol.InternalState. Moved inits from HTTPURLProtocol to NativeProtocol.Moved easyHandle and tempFileURL instance variables to NativeProtocol.EasyHandleDelegate methods provides functionality for transfer irrespective of protocol to NativeProtocol- fill(writeBuffer buffer: UnsafeMutableBufferPointer) , notifyDelegate(aboutUploadedData), notifyDelegate(aboutReceivedData), didReceive(data). transferCompleted(withError error: NSError?) createTransferState() createTransferBodyDataDrain However few methods like below are protocol specific hence have been overridden in respective protocol classes . didReceive(headerData data: Data, contentLength: Int64) Though the headers basically ends with \r\n irrespective of protocol the header format in FTP is different from HTTP. For e.g.: Headers in FTP looks like a plain text header with status code and description unlike the key-value pair headers in HTTP. NativeProtocol has definition for configureEasyHandle(easyHandle) which needs concrete implementation which is protocol specific by sub-classes like FTP and HTTP. For e.g.:HTTP requires some more additional configurations like adding the headers, setting allowed protocols. Moved HTTPURLProtocol. _ParsedResponseHeader from HTTPMessage.swift to NativeProtocol.swift under _NativeProtocol._ParsedResponseHeader . This implementation basically does header parsing based on \r\n delimiters. Except the fact that empty header is received in case of HTTP to mark as Header completion and 150 status marks the open of data communication in FTP New Implementation : NativeProtocol.validateHeaderComplete Should not throw error incase of simple-responseshttps://github.com/swiftlang/pull/1097 isCompleteHeader -> Inner method In order to support both protocols this method has been introduced -> which returns to true when the header is empty in case of HTTP. -> Returns true if the header starts with 150

Moved HTTPURLProtocol. _ParsedResponseHeader from HTTPMessage.swift to NativeProtocol.swift under _NativeProtocol._ParsedResponseHeader. This implementation basically does header parsing based on \r\n delimiters. Except the fact that empty header is received in case of HTTP to mark as Header completion and 150 status marks the open of data communication in FTP.

Moved methods specific to EasyHandle,common variables and common initializers to NativeProtocol.swift. Also overridden methods like configureEasyHandle which are specific to HTTP

saiHemak · 2017-11-10T09:23:59Z

@alblue @pushkarnk @xwu I have split the commit ,please review

alblue · 2017-11-10T11:32:34Z

The delete/add commits (64429de and a44a9f9) should really be one commit, not two. That way git will be able to figure out that there has been a move between files, rather than a mass-delete and mass-add. In fact, it might be better to spin that combined commit (of 64429de and a44a9f9) into its own separate review first of all, rebased against master. Once that can be verified it's not breaking any tests on its own, it can be merged - and then the body of this PR can be rebased upon it and we can review the changes subsequently.

So, I recommend:

cherry pick (and squash) 64429de and a44a9f9 onto master
create a new PR with that combined commit
get that reviewed and merged
revisit the changes on this PR with that base line

Sound good?

saiHemak · 2017-11-16T06:05:55Z

@alblue : Sure

xwu · 2017-11-16T06:17:56Z

Likewise subsequent pairs of commits that move code. The point is to have git demonstrate that each such a change is only moving code, never adding or deleting anything. It is not possible to see this on inspection when one commit removes the code and another adds it back.

alblue · 2017-11-22T12:01:34Z

Since we're breaking this apart and filing separate changes, I'm going to close this request as is. Will be happy to look at the incremental changes (such as the one merged previously) in order to get the functionality in. Thanks for keeping up with this!

pushkarnk requested a review from weissi September 14, 2017 10:09

weissi reviewed Sep 14, 2017

View reviewed changes

saiHemak force-pushed the urlsession-refactor branch from 8b5ad15 to 17b0be5 Compare September 15, 2017 09:41

pushkarnk requested a review from ianpartridge September 15, 2017 09:49

saiHemak force-pushed the urlsession-refactor branch from 17b0be5 to 09be77d Compare September 15, 2017 10:11

ianpartridge suggested changes Sep 15, 2017

View reviewed changes

saiHemak force-pushed the urlsession-refactor branch from 09be77d to 4060d34 Compare September 15, 2017 14:50

saiHemak mentioned this pull request Sep 15, 2017

Initial implementation of FTP #1122

Closed

saiHemak force-pushed the urlsession-refactor branch 2 times, most recently from 70b606f to 1cae8dd Compare September 18, 2017 11:18

saiHemak force-pushed the urlsession-refactor branch 3 times, most recently from ad85d11 to cf84a25 Compare September 26, 2017 07:27

saiHemak force-pushed the urlsession-refactor branch from cf84a25 to ddbad6d Compare October 3, 2017 08:50

saiHemak force-pushed the urlsession-refactor branch from ddbad6d to 4dca8bb Compare October 10, 2017 05:39

saiHemak force-pushed the urlsession-refactor branch 2 times, most recently from 9661d13 to ab72c5e Compare October 12, 2017 10:44

pushkarnk reviewed Oct 13, 2017

View reviewed changes

saiHemak force-pushed the urlsession-refactor branch from 26610ea to 5dbb4ba Compare October 31, 2017 06:23

saiHemak force-pushed the urlsession-refactor branch 3 times, most recently from 019eb13 to 349d2cf Compare November 10, 2017 08:39

saiHemak added 10 commits November 10, 2017 14:35

Add EasyHandle,MultiHandle and libcurlHelpers to a common libcurl dir…

a44a9f9

…ectory Added libcurl abstractions to libcurl directory

Common BodySource for native protocols

b2faece

The code respective to data handling is same irrespective to protocol.So moving it under URLSession folder.

Adding BodySource for native protocols

2e6330d

Moved under URLSession as the code related to data handling like read backed up by a file or buffer is common to all protocols.

Code moved to NativeProtocol

c2018e5

Transfer State has 1-0-1 relationship with Easy handle and is not bound to any protocol.So moved the code to NativeProtocol and renamed to _NativeProtocol. TransferState instead of HTTPTranserState

Refactored common code out of HTTPURLProtocol.swift

ac1d667

Moved methods specific to EasyHandle,common variables and common initializers to NativeProtocol.swift. Also overridden methods like configureEasyHandle which are specific to HTTP

build file changes

701cd1b

PR1310 changes

976f82b

saiHemak force-pushed the urlsession-refactor branch from 349d2cf to 976f82b Compare November 10, 2017 09:12

PR1311 changes

e01c498

saiHemak mentioned this pull request Nov 20, 2017

Added libcurl abstractions to libcurl directory #1329

Merged

alblue closed this Nov 22, 2017

saiHemak mentioned this pull request Nov 27, 2017

Moving HTTPBodySource out of http folder and renaming to BodySource #1336

Merged

saiHemak deleted the urlsession-refactor branch December 22, 2017 08:10

saiHemak restored the urlsession-refactor branch December 22, 2017 08:10

Refactoring URLSession for multiple native protocols #1216

Refactoring URLSession for multiple native protocols #1216

Uh oh!

Conversation

saiHemak commented Sep 14, 2017

Uh oh!

saiHemak commented Sep 14, 2017

Uh oh!

weissi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saiHemak Sep 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saiHemak Sep 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saiHemak commented Sep 15, 2017

Uh oh!

pushkarnk commented Sep 15, 2017

Uh oh!

pushkarnk commented Sep 15, 2017

Uh oh!

ianpartridge commented Sep 15, 2017

Uh oh!

alblue commented Sep 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ianpartridge commented Sep 15, 2017

Uh oh!

pushkarnk commented Sep 18, 2017

Uh oh!

ianpartridge commented Sep 18, 2017

Uh oh!

johnno1962 commented Oct 7, 2017

Uh oh!

saiHemak commented Oct 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saiHemak commented Oct 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xwu commented Nov 1, 2017

Uh oh!

saiHemak commented Nov 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alblue commented Nov 2, 2017

Uh oh!

saiHemak commented Nov 10, 2017

saiHemak Sep 15, 2017 •

edited

Loading

saiHemak Sep 15, 2017 •

edited

Loading

saiHemak commented Oct 31, 2017 •

edited

Loading

saiHemak commented Nov 2, 2017 •

edited

Loading